Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue 9: Getting started vignette #32

Merged
merged 19 commits into from
May 23, 2024
Merged

Issue 9: Getting started vignette #32

merged 19 commits into from
May 23, 2024

Conversation

athowes
Copy link
Collaborator

@athowes athowes commented May 14, 2024 •
edited
Loading

Description

This PR will close #9.
It is a draft of a rewritten version of the first "Get started" vignette.

Checklist

  • My PR is based on a package issue and I have explicitly linked it.
  • I have included the target issue or issues in the PR title in the for Issue(s) issue-numbers: PR title
  • I have read the contribution guidelines.
  • I have tested my changes locally.
  • I have added or updated unit tests where necessary.
  • I have updated the documentation if required.
  • My code follows the established coding standards.
  • I have added a news item linked to this PR.
  • I have reviewed CI checks for this PR and addressed them as far as I am able.

@seabbs
Copy link
Contributor

seabbs commented May 14, 2024

This looks good so far. Give me a ping when you want a proper review

@athowes
Copy link
Collaborator Author

athowes commented May 17, 2024

@seabbs @kgostic -- I'm not fully done with this but would be interested in some review / feedback to make sure that I'm staying on the right path.

If you'd prefer to wait until the vignette is more fully complete, then let me know.

@athowes athowes mentioned this pull request May 20, 2024
Closed
@athowes
Copy link
Collaborator Author

athowes commented May 20, 2024

I will now try fixing this pkgdown error so that you should hopefully be able to extract out the .html version of the vignette.

@athowes athowes force-pushed the first-vignette branch 2 times, most recently from cf51125 to d3cc70a Compare May 20, 2024 15:57
@athowes
Copy link
Collaborator Author

athowes commented May 20, 2024

I don't know what is causing the issue with the pkgdown website on check. I can render it OK locally. Let me know if you have any idea. It is also there on the other PR so it must be something already in main.

@kgostic
Copy link
Collaborator

kgostic commented May 20, 2024

@seabbs -- can you help with this tomorrow?

kgostic
kgostic previously approved these changes May 20, 2024
View reviewed changes
Copy link
Collaborator

@kgostic kgostic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@athowes this looks great overall. Your writing is really clear and concise.

I do think there's a bit at the end that still needs to be fleshed out, but if you can finish off the end, I think this is a reasonable outline for a getting started vignette.

@seabbs may want to add a bit more about installation and contributing issues at the top.

I'm approving so as not to hold things up but it would be really nice to see a rendered version of the makrdown as I haven't yet seen the figures in situ.

vignettes/epidist.Rmd Show resolved Hide resolved
vignettes/epidist.Rmd Outdated Show resolved Hide resolved
vignettes/epidist.Rmd Outdated Show resolved Hide resolved
vignettes/epidist.Rmd Outdated Show resolved Hide resolved
vignettes/epidist.Rmd Show resolved Hide resolved
vignettes/epidist.Rmd Outdated Show resolved Hide resolved
@seabbs
Copy link
Contributor

seabbs commented May 21, 2024

Can you rebase this so the pkgdown builds and then I will review (I thought about rebasing myself but it seemed a bit cheeky)

@athowes
Copy link
Collaborator Author

athowes commented May 21, 2024

@seabbs may want to add a bit more about installation and contributing issues at the top.

Agree on making sure people have the package installed, but would be tempted to have that displayed very prominently in the README and not in the "getting started" vignette. As well, FWIW and could be convinced, but I don't guess that the top of this vignette is the right place for anything long about contributing issues. Could be open to a short one sentence about it.

@seabbs
Copy link
Contributor

seabbs commented May 21, 2024 •
edited by athowes
Loading

Agree on making sure people have the package installed, but would be tempted to have that displayed very prominently in the README and not in the "getting started" vignette.

Agree

@athowes athowes changed the title Issue 9: Rewrite first vignette Issue 9: Getting started vignette May 21, 2024
seabbs
seabbs reviewed May 21, 2024
View reviewed changes
vignettes/epidist.Rmd Outdated Show resolved Hide resolved
seabbs
seabbs reviewed May 21, 2024
View reviewed changes
vignettes/epidist.Rmd Outdated Show resolved Hide resolved
seabbs
seabbs reviewed May 21, 2024
View reviewed changes
vignettes/epidist.Rmd Outdated Show resolved Hide resolved
seabbs
seabbs reviewed May 21, 2024
View reviewed changes
vignettes/epidist.Rmd Outdated Show resolved Hide resolved
seabbs
seabbs reviewed May 21, 2024
View reviewed changes
vignettes/epidist.Rmd Outdated Show resolved Hide resolved
seabbs
seabbs reviewed May 21, 2024
View reviewed changes
vignettes/epidist.Rmd Outdated Show resolved Hide resolved
seabbs
seabbs reviewed May 21, 2024
View reviewed changes
vignettes/epidist.Rmd Outdated Show resolved Hide resolved
seabbs
seabbs previously approved these changes May 21, 2024
View reviewed changes
Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this @athowes it is really clearly written and communicates the points very well.

I put a few minor comments. My only other concern here is maybe length but uncertain.

@seabbs
Copy link
Contributor

seabbs commented May 23, 2024

When ready for another review cna you reping using the interface at top right (as this gives a different form of notification that I filter for vs saying something like ready for review)

@athowes
Copy link
Collaborator Author

athowes commented May 23, 2024

Great, thanks all for the comments both.

I'm going to give this another hour or so try, and then I think we should clear it up and merge it in to get things moving. Happy to come back and iterate it again in future.

Have enjoyed writing this and the comments / feedback process by the way! Very smooth / logistically easy to handle.

@seabbs
Copy link
Contributor

seabbs commented May 23, 2024 •
edited
Loading

Actions points I see are:

  • Decide on if telling people about packages: did tell people
  • Add gt to the suggests: done
  • Decide on use of posterior vs from scratch output extraction: I don't know how to do it in posterior so doing from scratch
  • New issue for looking again at plotting to either use package plots, remove package plots or standardise themes: created issue Update figures in vignettes #55
  • Move @athowes from a contributor to an author in the DESCRIPTION: athowes is already "aut" (?)
  • Close all resolved conversations with clarification if useful: done, including creating issue Add right truncation figure to getting started vignette #56
  • Fix linting issues
  • Make ready for review and ping

@athowes athowes mentioned this pull request May 23, 2024
Open
@seabbs seabbs marked this pull request as ready for review May 23, 2024 19:52
seabbs
seabbs approved these changes May 23, 2024
View reviewed changes
Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. This looks to have covered all review points from myself and @kgostic.

@seabbs seabbs added this pull request to the merge queue May 23, 2024
Merged via the queue into main with commit f59b96e May 23, 2024
3 of 4 checks passed
@seabbs seabbs deleted the first-vignette branch May 23, 2024 19:56
seabbs pushed a commit that referenced this pull request Jan 10, 2025
@athowes
Issue 9: Getting started vignette (#32)
a4413cb 
* Rewriting and scoping out getting started vignette

* Basic version of code functionality

* Rewrite data simulation section

* Split up data observation process writing

* Rewrite Section 1 on data simulation

* Start writing the fitting section, and edits to Section 1

* Add epidemic growth figure

* Add plot of lognormal

* Add delays figure (and move to every 50th case)

* Improve figures here

* Fix lint issues

* Change from html_vignette2 to html_document2

* Change to fig.path

* Fixes based on Katie comments

* Work on the modelling section writing

* Improvements to first vignette based on Sam feedback

* Fix args problem, first go at results in at the end

* Resolve some remaining issues

* Fix lint issues

Former-commit-id: 125b206e3480fcdbd4471b33b967beb32c2f1f4e [formerly 6da89db91761e0693f7ba9d0af28f59d5fc42bff]
Former-commit-id: 43091c955bdb0cbfa15e0f5a88befe3bdafa1001
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seabbs seabbs seabbs approved these changes

@kgostic kgostic kgostic left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Expand getting started vignette
3 participants
@athowes @seabbs @kgostic